Skip to content

fix: correct port binding for non-default ports#112

Open
anisaoshafi wants to merge 4 commits intomainfrom
drg-611
Open

fix: correct port binding for non-default ports#112
anisaoshafi wants to merge 4 commits intomainfrom
drg-611

Conversation

@anisaoshafi
Copy link
Contributor

@anisaoshafi anisaoshafi commented Mar 12, 2026

When configuring a non-default port (e.g. 4567), the Docker binding was 4567:4567 instead of 4567:4566, so the health check could not reach localstack and lstk would hang.

Fix port binding for non-default ports so the emulator actually becomes ready.

@anisaoshafi anisaoshafi marked this pull request as ready for review March 12, 2026 18:10
@coderabbitai
Copy link

coderabbitai bot commented Mar 12, 2026

📝 Walkthrough

Walkthrough

Added an integration test that starts the service with a non-default port via a generated TOML config and mock license server. The docker runtime was changed to bind a fixed container port ("4566/tcp") instead of deriving the container port from the configured host port.

Changes

Cohort / File(s) Summary
Test Integration
test/integration/start_test.go
Added TestStartCommandSucceedsWithNonDefaultPort which writes a temporary TOML config (port 4567), starts a mock license server, and runs lstk start --config to verify successful startup.
Docker Runtime
internal/runtime/docker.go
Introduced emulatorContainerPort = nat.Port("4566/tcp") and updated Start to use this fixed container port for ExposedPorts and PortBindings instead of deriving from config.Port.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: correcting port binding for non-default ports in Docker configuration.
Description check ✅ Passed The pull request description clearly explains the bug fix for Docker port binding when using non-default ports and how the changes resolve the issue.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch drg-611
📝 Coding Plan
  • Generate coding plan for human review comments

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
internal/container/start.go (1)

326-332: Consider extracting the default port to a constant to avoid duplication.

The string "4566" is hardcoded here, but the same default is also defined in internal/config/config.go:setDefaults(). If the default port changes in one place but not the other, non-default port detection will silently break.

♻️ Suggested approach

Define a constant in the config package and reference it from both locations:

// In internal/config/config.go
const DefaultPort = "4566"

func setDefaults() {
    viper.SetDefault("containers", []map[string]any{
        {
            "type": "aws",
            "tag":  "latest",
            "port": DefaultPort,
        },
    })
}

Then in needsGatewayListen:

 func needsGatewayListen(port string, env []string) bool {
-    return port != "4566" && !slices.ContainsFunc(env, func(e string) bool {
+    return port != config.DefaultPort && !slices.ContainsFunc(env, func(e string) bool {
         return strings.HasPrefix(e, "GATEWAY_LISTEN=")
     })
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/container/start.go` around lines 326 - 332, Extract the hardcoded
"4566" into a shared constant (e.g. DefaultPort string) in the config package
(add const DefaultPort = "4566" in internal/config/config.go and use it inside
setDefaults()), then replace the literal "4566" in needsGatewayListen with
config.DefaultPort and add the config import in internal/container/start.go;
ensure all references to the default port use config.DefaultPort so non-default
detection remains consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@internal/container/start.go`:
- Around line 326-332: Extract the hardcoded "4566" into a shared constant (e.g.
DefaultPort string) in the config package (add const DefaultPort = "4566" in
internal/config/config.go and use it inside setDefaults()), then replace the
literal "4566" in needsGatewayListen with config.DefaultPort and add the config
import in internal/container/start.go; ensure all references to the default port
use config.DefaultPort so non-default detection remains consistent.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 08836855-fa8f-4c4d-8368-99ea054603a3

📥 Commits

Reviewing files that changed from the base of the PR and between 2ce11ae and ce6eef2.

📒 Files selected for processing (2)
  • internal/container/start.go
  • test/integration/start_test.go

@anisaoshafi anisaoshafi changed the title fix: pass GATEWAY_LISTEN env var for non-default ports fix: correct port binding for non-default ports Mar 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant